Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DBAL-630] Incorrect PostgreSQL boolean handling #564

Closed
wants to merge 13 commits into from

Conversation

davividal
Copy link
Contributor

Working on PostgreSQL incorrect boolean handling when emulating prepared statements

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-863

We use Jira to track the state of pull requests and the versions they got
included in.

@davividal
Copy link
Contributor Author

The only issue with this PR is that it breaks Doctrine2 ORM. I already have a patch for that too, but I'm not sure on how to proceed.

* @return mixed
*/
public function convertBooleans($item)
public function convertBoolToSqlLiteral($item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change the public API here, this breaks BC and is unacceptable.

*/
public function convertBoolToDbValue($item)
{
return self::convertBooleans($item);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use $this->, not self:: as you are refering to an instance method, not to a static method

@davividal
Copy link
Contributor Author

Any more thoughts on this?

$item = ($item) ? 'true' : 'false';
} elseif (is_string($item)) {
$item = trim(strtolower($item));
if ('false' !== $item && 'f' !== $item) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it maybe necessary to take the other valid boolean literals like n, no, off etc (and opposites) into account which are mentioned in the documentation? I'm not sure about this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought about it.

What do you think about a private attribute called $booleansLiterals?

$booleansLiterals = ['true' => ['t', 'true', ...], 'false' => ['f', 'false', ...]];

@deeky666
Copy link
Member

deeky666 commented Apr 9, 2014

Btw, is this PR complementary to #527 ?

@davividal
Copy link
Contributor Author

@deeky666 it looks like this PR complements #527 . How should I proceed?

@@ -716,14 +738,62 @@ public function convertBooleans($item)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're not using TrueFalseStrings, convertBooleans('t') won't convert it to neither 1 nor 0.
The same goes for almost every case described in PostgreSQL documentation.

@davividal
Copy link
Contributor Author

@deeky666 @stof any other thoughts on this?

return parent::convertBooleansToDatabaseValue($item);
}

if (is_array($item)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole if block is exactly the same as in convertBooleans() except for the values of true and false. Would be nice to avoid code duplication through another private method here IMO.

@deeky666
Copy link
Member

Despite my suggested improvements I am fine with this PR but TBH I don't have a clue about emulated prepared statements in PostgreSQL and I would like to hear someone else's meaning about the validity of this fix. Otherwise it looks reasonable to me.

@deeky666
Copy link
Member

@beberlei you have made the first step on this issue. Thoughts?

@davividal
Copy link
Contributor Author

@deeky666 I did a refactoring, what do you think?

@deeky666
Copy link
Member

not quite what I thought. Here is my suggestion (not tested):

/**
 * Converts a single boolean value.
 *
 * First converts the value to its native PHP boolean type
 * and passes it to the given callback function to be reconverted
 * into any custom representation.
 *
 * @param mixed    $value    The value to convert.
 * @param callable $callback The callback function to use for converting the real boolean value.
 *
 * @return mixed
 */
private function convertSingleBooleanValue($value, $callback)
{
    if (null === $value) {
        return $callback(false);
    }

    if (is_bool($value) || is_numeric($value)) {
        return $callback($value ? true : false);
    }

    if (is_string($value) && in_array(trim(strtolower($value)), $this->booleanLiterals['false'])) {
        return $callback(false);
    }

    return $callback(true);
}

/**
 * Converts one or multiple boolean values.
 *
 * First converts the value(s) to their native PHP boolean type
 * and passes them to the given callback function to be reconverted
 * into any custom representation.
 *
 * @param  $item     The value(s) to convert.
 * @param  $callback The callback function to use for converting the real boolean value(s).
 *
 * @return mixed
 */
private function doConvertBooleans($item, $callback)
{
    if (is_array($item)) {
        foreach ($item as $key => $value) {
            $item[$key] = $this->convertSingleBooleanValue($value, $callback);
        }

        return $item;
    }

    return $this->convertSingleBooleanValue($item, $callback);
}

/**
 * {@inheritdoc}
 */
public function convertBooleans($item)
{
    if ( ! $this->useBooleanTrueFalseStrings) {
        return parent::convertBooleans($item);
    }

    return $this->doConvertBooleans(
        $item,
        function ($boolean) {
            return true === $boolean ? 'true' : 'false';
        }
    );
}

/**
 * {@inheritdoc}
 */
public function convertBooleansToDatabaseValue($item)
{
    if ( ! $this->useBooleanTrueFalseStrings) {
        return parent::convertBooleansToDatabaseValue($item);
    }

    return $this->doConvertBooleans(
        $item,
        function ($boolean) {
            return (int) $boolean;
        }
    );
}

@davividal
Copy link
Contributor Author

@deeky666 it works. Your solution is beautiful!

'off',
'0'
)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indentation. You have 4 spaces to much here from below the array opener. Also I wonder if we even need this map. If you look at your current implementation, we basically only DO and NEED TO check for values evaluating to false, no? Everything else being converted is true anyways. Like if I pass an object or a resource or anything else (stupid but possible) that is not null, false, 0, 'f', 'false', 'n', 'no', 'off' or '0'. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise if we would strictly stick tho this map we would have to throw exceptions for unexpected values which we cannot do IMO because that might break BC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Smart" indent, sorry.

I'm not sure. Initially, I was checking only for false values and converting everything else to true. But if I'm using Postgres boolean literals for true, I will use one of these six. If I'm inputing a literal for true different than these, I would expect an error.

This is Postgres behavior btw:

ERROR:  invalid input syntax for type boolean: "my-awesome-true"
LINE 1: SELECT 'my-awesome-true'::BOOLEAN

As for what do I think: I really think that false should be equal to 0, false or null, true otherwise, like C. Diverging from that you get Ruby awesome 0 equals to true. :) [/troll]

@deeky666
Copy link
Member

@Ocramius can you please have a look at this implementation. I forgot to keep in mind that those methods could be performance sensitive due being used in DBAL type conversion. Do you think we have to undDRY this thing again?

@davividal
Copy link
Contributor Author

@deeky666 @Ocramius @stof can I help with anything to get this merged?

@deeky666
Copy link
Member

deeky666 commented May 5, 2014

Still waiting for feedback on possible performance implications.
@Ocramius ping

@davividal
Copy link
Contributor Author

Please let me know if I can help with some benchmarking.

@deeky666
Copy link
Member

deeky666 commented May 5, 2014

@davividal Sure go ahead and do so. Then we don't have to speculate about that :) Basically the conversion via BooleanType is important to know about.

@davividal
Copy link
Contributor Author

Sorry, I wasn't able to do any progress on this.

If anyone could help, I don't know when I will be able to (re-)take this.

* @return mixed
*/
private function convertSingleBooleanValue($value, $callback)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce nested complexity this can be rewritten as:

if (null === $value) {
    return $callback(false);
}

if (is_bool($value) || is_numeric($value)) {
    return $callback($value ? true : false);
}

if (!is_string($value)) {
    return $callback(true);
}

/**
 * Better safe than sorry: http://php.net/in_array#106319
 */
if (in_array(trim(strtolower($value)), $this->booleanLiterals['false'], true)) {
    return $callback(false);
} 

if (in_array(trim(strtolower($value)), $this->booleanLiterals['true'], true)) {
    return $callback(true);
}

throw new \UnexpectedValueException("Unrecognized boolean literal '${value}'");

@lucasvanlierop
Copy link
Contributor

I've been comparing this PR with the #527 which seems almost finished. And I feel it would be best if that PR is finished and merged soon and the result is merged into this one. It does not do exactly the same but there is overlap and I think it's best if conversion in both directions both work more or less the same way.

Kudos to Lucas van Lierop for the refactoring
@Ocramius
Copy link
Member

Closing - continuing in #625

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants